-
Notifications
You must be signed in to change notification settings - Fork 600
fix: missing thiserror crate when building with --no-default-features
#2408
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2408 +/- ##
=====================================
Coverage 79.3% 79.3%
=====================================
Files 123 123
Lines 21566 21566
=====================================
Hits 17112 17112
Misses 4454 4454 ☔ View full report in Codecov by Sentry. |
| futures-sink = { version = "0.3", optional = true } | ||
| pin-project-lite = { workspace = true, optional = true } | ||
| thiserror = { workspace = true, optional = true} | ||
| thiserror = { workspace = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to keep optional dependency on thiserror. As of now, it is supposed to be used only when trace feature flag is enabled. If that is not the case, we should fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thiserror is used in opentelemetry::propagation. So it should be removed from there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
propagators are only used in the context of traces. See if we can include it only if trace flag is enabled here -
opentelemetry-rust/opentelemetry/src/lib.rs
Line 267 in 2573773
| pub mod propagation; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and also, you need to sign the CLA in order for this to be merged :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's fine for you I would prefer to not sign the CLA and leave the fix to you? 😅 Thanks for getting back so quickly!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Thanks for the issue :)
|
closing this in favor of #2413 |
Building with
cargo check -p opentelemetry --no-default-featuresfails sincethiserroris optional.Changes
Please provide a brief description of the changes here.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes